Skip to content

Conversation

vladislav-orlovskiy
Copy link
Contributor

…sal across partitions.

Description

according to this nice gentelmen list i was able to find that rds service principal for china is the same as for commertial region (not sure about AWS gov tho)

Motivation and Context

policy simply not work in China because service pricipal is incorrect.

Breaking Changes

I think not since this part was unusable in China anyway.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

Co-authored-by: Bryant Biggs <[email protected]>
@vladislav-orlovskiy
Copy link
Contributor Author

vladislav-orlovskiy commented May 15, 2025

@bryantbiggs can we use the following approach? I think it would be much more reliable and native

data "aws_region" "current" {}
data "aws_service_principal" "rds" {
  service_name = "rds"
  region = data.aws_region.current.region
}
identifiers = [data.aws_service_principal.rds.id]

I updated the code. Please take a look and let me know if that ok with you.

@bryantbiggs
Copy link
Member

@bryantbiggs can we use the following approach? I think it would be much more reliable and native

data "aws_region" "current" {}
data "aws_service_principal" "rds" {
  service_name = "rds"
  region = data.aws_region.current.region
}
identifiers = [data.aws_service_principal.rds.id]

please let me know. and if its ok i'll update my PR.

yes, that looks great. minor tweak to ensure we create nothing when create = false

data "aws_region" "current" {
  count = var.create ? 1 : 0
}
data "aws_service_principal" "rds" {
  count = var.create ? 1 : 0

  service_name = "rds"
  region = data.aws_region.current[0].region
}

@vladislav-orlovskiy
Copy link
Contributor Author

@bryantbiggs can we use the following approach? I think it would be much more reliable and native

data "aws_region" "current" {}
data "aws_service_principal" "rds" {
  service_name = "rds"
  region = data.aws_region.current.region
}
identifiers = [data.aws_service_principal.rds.id]

please let me know. and if its ok i'll update my PR.

yes, that looks great. minor tweak to ensure we create nothing when create = false

data "aws_region" "current" {
  count = var.create ? 1 : 0
}
data "aws_service_principal" "rds" {
  count = var.create ? 1 : 0

  service_name = "rds"
  region = data.aws_region.current[0].region
}

done. however i did not added count to aws_region resource since its being mentioned by different resources already. I leaved it as is.

@vladislav-orlovskiy
Copy link
Contributor Author

vladislav-orlovskiy commented May 15, 2025

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/service_principal
still gives an incorrect principal for RDS.

 operation error IAM: CreateRole, https response error StatusCode: 400, RequestID: 4a165567-08b8-40f7-99b4-a45fb1daa725, MalformedPolicyDocument: Invalid principal in policy: \\"SERVICE\\":\\"rds.cn-northwest-1.amazonaws.com\\"","detail":"","address":"module.complete.module.rds_proxy.aws_iam_role.this[0]","range":{"filename":".terraform/modules/complete.rds_proxy/main.tf","start":{"line":123,"column":32,"byte":4109},"end":{"line":123,"column":33,"byte":4110}},"snippet":{"context":"resource \\"aws_iam_role\\" \\"this\\"","code":"resource \\"aws_iam_role\\" \\"this\\" {","start_line":123,"highlight_start_offset":31,"highlight_end_offset":32,"values":[]}},"type":"diagnostic"}\n::error::Terraform exited with code 1.\n ', '')

however it gives the correct suffix for China
issue is created in aws provider hashicorp/terraform-provider-aws#42642
however it blocks us from using this module right now in china
my suggestion is to use this datasource like that.

    principals {
      type        = "Service"
      identifiers = ["rds.${data.aws_service_principal.rds[0].suffix}"]
    }

i tested that just now and it workf flawlesly in China and in AWS partition.
Sadly I dont have an account in aws-gov to test it out.
@bryantbiggs ^

@vladislav-orlovskiy
Copy link
Contributor Author

@antonbabenko ^

@vladislav-orlovskiy
Copy link
Contributor Author

vladislav-orlovskiy commented May 19, 2025

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/service_principal still gives an incorrect principal for RDS.

 operation error IAM: CreateRole, https response error StatusCode: 400, RequestID: 4a165567-08b8-40f7-99b4-a45fb1daa725, MalformedPolicyDocument: Invalid principal in policy: \\"SERVICE\\":\\"rds.cn-northwest-1.amazonaws.com\\"","detail":"","address":"module.complete.module.rds_proxy.aws_iam_role.this[0]","range":{"filename":".terraform/modules/complete.rds_proxy/main.tf","start":{"line":123,"column":32,"byte":4109},"end":{"line":123,"column":33,"byte":4110}},"snippet":{"context":"resource \\"aws_iam_role\\" \\"this\\"","code":"resource \\"aws_iam_role\\" \\"this\\" {","start_line":123,"highlight_start_offset":31,"highlight_end_offset":32,"values":[]}},"type":"diagnostic"}\n::error::Terraform exited with code 1.\n ', '')

however it gives the correct suffix for China issue is created in aws provider hashicorp/terraform-provider-aws#42642 however it blocks us from using this module right now in china my suggestion is to use this datasource like that.

    principals {
      type        = "Service"
      identifiers = ["rds.${data.aws_service_principal.rds[0].suffix}"]
    }

i tested that just now and it workf flawlesly in China and in AWS partition. Sadly I dont have an account in aws-gov to test it out. @bryantbiggs ^

@bryantbiggs sorry i was wrong. I didnt read the resource trough
it supposed to be

data.aws_service_principal.rds[0].name

i changed that. its working. please take a look when you have time.
so sorry for all the confusion. I will be more careful next time.

@vladislav-orlovskiy
Copy link
Contributor Author

@bryantbiggs
kindly ping.

@antonbabenko antonbabenko changed the title fix: Correct service principal to rds.amazonaws.com since ints univer… fix: Correct service principal to rds.amazonaws.com (incl China) May 22, 2025
@antonbabenko antonbabenko merged commit bbbf50c into terraform-aws-modules:master May 22, 2025
9 checks passed
antonbabenko pushed a commit that referenced this pull request May 22, 2025
## [3.2.1](v3.2.0...v3.2.1) (2025-05-22)

### Bug Fixes

* Correct service principal to rds.amazonaws.com (incl China) ([#32](#32)) ([bbbf50c](bbbf50c))
@antonbabenko
Copy link
Member

This PR is included in version 3.2.1 🎉

@vladislav-orlovskiy
Copy link
Contributor Author

@antonbabenko thanks a lot for stepping in!
btw will new version be published automatically on https://registry.terraform.io/modules/terraform-aws-modules/rds-proxy/aws/latest or we have to wait a bit?

@antonbabenko
Copy link
Member

Fixed.

Hashicorp has released some changes to the Terraform Registry integration, which was not stable before and doesn't seem to be stable now either.

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants